Conversation
Replaces the previous strategy of each of the signer traits providing two methods: 1. a fallible method named `try_*` that returns `signature::Result` with `signature::Error` in the `E` position. 2. an infallible equivalent method that panics if the `try_*` method returns an error. In its place, each of the signing traits has been split into an infallible trait and a `Try*`-prefixed fallible trait, e.g. `Signer`/`TrySigner`, `RandomizedSigner`/`TryRandomizedSigner`. The fallible traits now each have an associated `Error` type. The previous support for sneaking an inner error type through `signature::Error` has been removed, and it's now a simple unit struct. Where it makes sense, the infallible traits bound on the respective `Try*` traits with `Error = Infallible`, and a blanket impl is provided, e.g. for `TrySigner` and `TryMultipartSigner`. This isn't always possible though, e.g. `RandomizedSigner` uses `CryptoRng` for `sign_with_rng`, whereas `TryRandomizedSigner` uses `TryCryptoRng`. Likewise, `DigestSigner` uses a callback which returns `()`, whereas `TryDigestSigner` returns `Result<(), Error>`. The main advantages of this approach are that signing operations are for most common use cases infallible, and this approach eliminates the panic condition for such cases, and avoids people who don't want to think about handling signing errors from having to. However, for the people using e.g. external/remote signers that can have I/O errors, this gives them a concrete error type to work with instead of trying to smuggle one through `signature::Error`. The main disadvantage is a doubling of the number of traits, where we already have a combinatorial explosition of possibilities.
|
I worry it's a lot of traits, even if any given one is cleaner: 
|
| @@ -25,60 +33,106 @@ pub trait Signer<S> { | |||
| /// # Errors | |||
| /// Returns implementation-specific errors in the event signing failed (e.g. KMS or HSM | |||
| /// communication error). | |||
| fn try_sign(&self, msg: &[u8]) -> Result<S, Error>; | |||
| fn try_sign(&self, msg: &[u8]) -> Result<S, Self::Error>; | |||
| } | |||
|
|
|||
| impl<K, S> TrySigner<S> for K | |||
| where | |||
| K: Signer<S>, | |||
| { | |||
| type Error = Infallible; | |||
|
|
|||
| fn try_sign(&self, msg: &[u8]) -> Result<S, Self::Error> { | |||
| Ok(self.sign(msg)) | |||
| } | |||
| } | |||
|
|
|||
| /// Equivalent of [`Signer`] but the message is provided in non-contiguous byte slices. | |||
| pub trait MultipartSigner<S> { | |||
| pub trait MultipartSigner<S>: TryMultipartSigner<S, Error = Infallible> { | |||
| /// Equivalent of [`Signer::sign()`] but the message is provided in non-contiguous byte slices. | |||
| fn multipart_sign(&self, msg: &[&[u8]]) -> S { | |||
| self.try_multipart_sign(msg) | |||
| .expect("signature operation failed") | |||
| } | |||
| fn multipart_sign(&self, msg: &[&[u8]]) -> S; | |||
| } | |||
|
|
|||
| /// Equivalent of [`Signer`] but the message is provided in non-contiguous byte slices. | |||
| /// | |||
| /// This is a fallible equivalent of the [`MultipartSigner`] trait. | |||
| pub trait TryMultipartSigner<S> { | |||
| /// Error type. | |||
| type Error: core::error::Error + Into<Error>; | |||
There was a problem hiding this comment.
The repetition of these error types is annoying and might be doubly so if multiple traits with them are in scope
|
In trying to open a PR to https://github.com/rustcrypto/signatures I noticed that pretty much all of our signature algorithms are internally fallible and rely on the existing behavior of panicking in conditions which have an infinitesimal chance of happening. For deterministic signature algorithms this fallibility should probably be replaced with retry logic which is generally supported by the deterministic signature algorithms but unimplemented. This is some nontrivial refactoring work that needs to be done on every single signature algorithm so the ones which can be implemented infallibly truly support an infallible API this crate can provide a wrapper around. In the intervening time, I don't think it makes sense to impose a fallible API. While I think a change like this would ultimately be good, I think it's too much work, too invasive/non-trivial, and far too late in the development cycle to ship as a last minute change. I will open a separate issue on https://github.com/rustcrypto/signatures about implementing the retry logic. |
1 participant
Replaces the previous strategy of each of the signer traits providing two methods:
try_*that returnssignature::Resultwithsignature::Errorin theEposition.try_*method returns an error.In its place, each of the signing traits has been split into an infallible trait and a
Try*-prefixed fallible trait, e.g.Signer/TrySigner,RandomizedSigner/TryRandomizedSigner.The fallible traits now each have an associated
Errortype. The previous support for sneaking an inner error type throughsignature::Errorhas been removed, and it's now a simple unit struct.Where it makes sense, the infallible traits bound on the respective
Try*traits withError = Infallible, and a blanket impl is provided, e.g. forTrySignerandTryMultipartSigner.This isn't always possible though, e.g.
RandomizedSignerusesCryptoRngforsign_with_rng, whereasTryRandomizedSignerusesTryCryptoRng.Likewise,
DigestSigneruses a callback which returns(), whereasTryDigestSignerreturnsResult<(), Error>.The main advantages of this approach are that signing operations are for most common use cases infallible, and this approach eliminates the panic condition for such cases, and avoids people who don't want to think about handling signing errors from having to. However, for the people using e.g. external/remote signers that can have I/O errors, this gives them a concrete error type to work with instead of trying to smuggle one through
signature::Error.The main disadvantage is a doubling of the number of traits, where we already have a combinatorial explosition of possibilities.